Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HTTPServerRequest: Build urlURL using URLComponents #143

Merged
merged 1 commit into from
Jan 18, 2019

Conversation

nethraravindran
Copy link
Contributor

This increases the performance by ~2% while accessingurlURL

@nethraravindran nethraravindran self-assigned this Jan 8, 2019
@pushkarnk
Copy link
Contributor

@nethraravindran Bridging UInt16 to NSNumber doesn't work pre-4.2. We have two options:

  1. replace it by NSNumber(value:) all throughout
  2. do (1) only for pre-4.2 compiler versions

I ran a small benchmark and find performance of NSNumber(value:) similar to x as NSNumber. So, we may adopt (1).

@nethraravindran
Copy link
Contributor Author

@pushkarnk I have addressed the changes. Please review. Thank you!

Copy link
Contributor

@pushkarnk pushkarnk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Performance apart, this seems like a better way of building urlURL.

@nethraravindran nethraravindran added this to the 2019.01 milestone Jan 9, 2019
Copy link
Contributor

@ianpartridge ianpartridge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no problem with this in principle, but we are creating a NSURLComponents() and throwing it away. We also have this property (line 43):

public var urlComponents: URLComponents

so could we cache the URLComponents we create and use it in that property? This all seems a bit messy at the moment.

@codecov-io
Copy link

codecov-io commented Jan 17, 2019

Codecov Report

Merging #143 into master will decrease coverage by 0.15%.
The diff coverage is 62.5%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #143      +/-   ##
==========================================
- Coverage   67.11%   66.95%   -0.16%     
==========================================
  Files          20       20              
  Lines        1271     1274       +3     
==========================================
  Hits          853      853              
- Misses        418      421       +3
Flag Coverage Δ
#KituraNet 66.95% <62.5%> (-0.16%) ⬇️
Impacted Files Coverage Δ
Sources/KituraNet/HTTP/HTTPServerRequest.swift 70.37% <62.5%> (-1.33%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 242c40a...46fbee0. Read the comment docs.

if let _urlComponents = _urlComponents {
return _urlComponents
} else {
return URLComponents(url: urlURL, resolvingAgainstBaseURL: false) ?? URLComponents()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nethraravindran We must cache it here as well, in the scenario where urlComponents was invoked before urlURL

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pushkarnk Sure!


self.enableSSL ? url.append("https://") : url.append("http://")
_urlComponents = URLComponents()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check if _urlComponents already exists here, we could have initialised it in urlComponents

@nethraravindran nethraravindran modified the milestones: 2019.01, 2019.02 Jan 17, 2019
@nethraravindran nethraravindran changed the title HTTPServerRequest: Build urlURL using NSURLComponents HTTPServerRequest: Build urlURL using URLComponents Jan 17, 2019
@pushkarnk pushkarnk merged commit 45d22b7 into Kitura:master Jan 18, 2019
@pushkarnk pushkarnk mentioned this pull request Jan 19, 2019
nethraravindran added a commit to nethraravindran/Kitura-NIO that referenced this pull request Jan 20, 2019
nethraravindran added a commit to nethraravindran/Kitura-NIO that referenced this pull request Jan 21, 2019
pushkarnk pushed a commit to nethraravindran/Kitura-NIO that referenced this pull request Jan 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants